Lax parser feature #363

cschramm · 2025-12-17T10:21:38Z

The parser strictly checks reserved areas to be zero (even though the specification does not even require that for all of them). This is an issue when parsing future (i.e., not known to the current codebase) versions of attestation reports, where those areas may no longer be reserved.

The lax-parser feature disables that check, currently with some tradeoffs:

No WriteExt::skip_bytes
No Encoder impl for AttestationReport, Signature, and WrappedVlekHashstick
No compatibility with openssl and crypto_nossl

Closes #343

The parser strictly checks reserved areas to be zero (even though the specification does not even require that for all of them). This is an issue when parsing newer versions of attestation reports where those areas might not be reserved anymore. The `lax-parser` feature disables that check, currently with some tradeoffs: * No `WriteExt::skip_bytes` * No `Encoder` impl for `AttestationReport`, `Signature`, and `WrappedVlekHashstick` * No compatibility with `openssl` and `crypto_nossl` Closes virtee#343 Signed-off-by: Christopher Schramm <[email protected]>

DGonzalezVillal · 2025-12-19T18:59:48Z

So in this solution, you would only be able to parse the attestation report, but you would not be able to write it anywhere correct? Is that still useful to you?

DGonzalezVillal · 2025-12-19T19:04:05Z

Because the Write stuff can be modified to write the raw contents. Create something like a raw write

cschramm · 2025-12-19T23:00:29Z

Is that still useful to you?

Absolutely, yes. I only use the parser, but not the Write stuff.

As I wrote in #343, writing could be achieved by actually reading reserved areas and carrying their contents to write them back instead of zeroes. I was under the impression, though, that you wanted to keep this a limited parse-only case.

DGonzalezVillal

Hello @cschramm,

Apologies for the delay—I was out on break.

After taking a closer look at your PR, I’m thinking a better approach might be to shift from the current lax-parsing idea to something more like a raw read/write (or unsafe read/write) mode. With that approach, this feature would simply read and write the raw binary contents as-is.

In our earlier discussions, I may have implied that I wanted this scoped strictly to parsing, but after reviewing the PR and realizing that lax parsing would require disabling or weakening other APIs, I’m less comfortable with that direction. I’d prefer not to reduce existing functionality to support a single feature. This alternative also results in fewer changes overall, since it would mainly involve updating the read and write paths to operate on raw bytes without safety checks.

This keeps the library behavior unchanged for most users, while clearly shifting responsibility for validation and safety checks to users who opt into this mode.

Please let me know what you think, and apologies again for the back and forth.

Thanks!

DGonzalezVillal · 2026-01-06T18:49:30Z

src/firmware/host/mod.rs

        Ok(())
    }

+    #[cfg(not(feature = "lax-parser"))]


Why do you have to disable this API for lax-parser?

It encodes hashstick, which is not supported as it requires skip_bytes.

DGonzalezVillal · 2026-01-06T18:51:56Z

src/lib.rs

+#[cfg(all(
+    any(feature = "openssl", feature = "crypto_nossl"),
+    feature = "lax-parser"
+))]


Why would lax-parser be part of this check?

This check is precisely to detect the use of one of the crypto features together with lax-parser. The only purpose is to give a clear error message above the error messages that will follow due to the disabled code.

cschramm

Hi @DGonzalezVillal, no worries. A lax parser that does not disable encoding support makes perfect sense to me. I'll prepare an alternative PR for that. 👍

Edit: => #366 Keeping this open for reference until the other direction is settled.

cschramm · 2026-01-07T13:11:19Z

src/firmware/host/mod.rs

        Ok(())
    }

+    #[cfg(not(feature = "lax-parser"))]


It encodes hashstick, which is not supported as it requires skip_bytes.

cschramm · 2026-01-07T13:15:01Z

src/lib.rs

+#[cfg(all(
+    any(feature = "openssl", feature = "crypto_nossl"),
+    feature = "lax-parser"
+))]


This check is precisely to detect the use of one of the crypto features together with lax-parser. The only purpose is to give a clear error message above the error messages that will follow due to the disabled code.

hyperfinitism · 2026-01-10T15:05:55Z

This looks like a very useful change overall, especially treating reserved fields in the top-level AttestationReport as raw bytes and relaxing the zero-check via a new feature. That aligns better with the ABI spec and improves forward compatibility.

One subtle point: in the ABI spec, some reserved fields are explicitly marked as MBZ (must be zero), while others are simply described as reserved without an MBZ requirement. In other words, the ABI does not guarantee that all reserved fields are zero-filled. From that perspective, it would be more spec-accurate to always zero-check only explicitly documented MBZ fields, and avoid zero-checking non-MBZ reserved fields even without enabling the feature.

The same issue also applies to sub-structures (e.g. TCB Version, Guest Policy, Platform Info), which still handle reserved fields in the old way.

As @DGonzalezVillal suggested:

a better approach might be to shift from the current lax-parsing idea to something more like a raw read/write (or unsafe read/write) mode

I strongly agree with this direction. A raw read/write model avoids baking ABI assumptions into parsing, and it naturally resolves the MBZ vs non-MBZ ambiguity (including for sub-structures) in a cleaner and more scalable way.

As additional context, similar strict-parsing assumptions have already caused issues downstream. For example:

Bump VirTEE/sev from 6.1.0 to 6.2.1 (fixes TCB serialization bug) snpguest#98 — mishandling TcbVersion variants (pre-Turin vs. Turin)
report command fails with "Skipped bytes were expected to be zeroed" snpguest#113 — zero-check failures triggered by newly added fields in V5 format

Both were ultimately related to parsing expectations around attestation report layout and reserved fields. A raw read/write (opaque binary) model would not only prevent this class of bugs, but also make root-cause analysis and fixes much simpler when ABI-level discrepancies occur.

cschramm · 2026-01-10T17:38:56Z

@hyperfinitism

One subtle point: in the ABI spec, some reserved fields are explicitly marked as MBZ (must be zero), while others are simply described as reserved without an MBZ requirement. In other words, the ABI does not guarantee that all reserved fields are zero-filled. From that perspective, it would be more spec-accurate to always zero-check only explicitly documented MBZ fields, and avoid zero-checking non-MBZ reserved fields even without enabling the feature.

I mentioned that in #343 (comment), but @DGonzalezVillal responded with

On reserved fields: you can assume they are zero in supported versions.

I do not understand why, but I'm definitely the one with less domain knowledge here.

In case you missed it: There is #366 with an alternative approach that reads raw bytes.

hyperfinitism · 2026-01-12T18:17:40Z

In case you missed it: There is #366 with an alternative approach that reads raw bytes.

Yes. I read it. The alternative design I have in mind is to store the attestation report as a 1184-byte blob while providing setters and getters for each field.

cschramm · 2026-01-15T15:58:31Z

I see. AttestationReport's Decoder would then use those getters? How would it apply the zero checks? Raw access (applying structural knowledge)? A method on the raw type dedicated to those checks? 🤔

Fine for me either. @DGonzalezVillal?

cschramm requested review from DGonzalezVillal, larrydewey and tylerfanelli as code owners December 17, 2025 10:21

auto-assign bot requested a review from ryansavino December 17, 2025 10:21

auto-assign bot assigned ryansavino and tylerfanelli Dec 17, 2025

cschramm force-pushed the lax-decoders branch from e6e308a to 629cd46 Compare December 17, 2025 10:24

cschramm force-pushed the lax-decoders branch from 629cd46 to e5c58d6 Compare December 17, 2025 10:26

cschramm mentioned this pull request Dec 17, 2025

Forward compatibility #343

Open

DGonzalezVillal requested changes Jan 6, 2026

View reviewed changes

cschramm commented Jan 7, 2026

View reviewed changes

Lax parser feature #363

Are you sure you want to change the base?

Lax parser feature #363

Uh oh!

Conversation

cschramm commented Dec 17, 2025

Uh oh!

DGonzalezVillal commented Dec 19, 2025

Uh oh!

DGonzalezVillal commented Dec 19, 2025

Uh oh!

cschramm commented Dec 19, 2025

Uh oh!

DGonzalezVillal left a comment

Choose a reason for hiding this comment

Uh oh!

DGonzalezVillal Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

cschramm Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

DGonzalezVillal Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

cschramm Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

cschramm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cschramm Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

cschramm Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

hyperfinitism commented Jan 10, 2026

Uh oh!

cschramm commented Jan 10, 2026

Uh oh!

hyperfinitism commented Jan 12, 2026

Uh oh!

cschramm commented Jan 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cschramm left a comment •

edited

Loading